-
-
Notifications
You must be signed in to change notification settings - Fork 544
The Plan, phase 1 #1341
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
The Plan, phase 1 #1341
Conversation
f2bd544
to
454a716
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in practice, it may be useful for us to
nest this intousers.users.*.homebrew
instead, at the expense of
being an unsupported setup if used to its full potential. Since
that would be a breaking change to the inteface anyway, I think
addinghomebrew.user
for now is acceptable.
I understand the motive here, but are you sure it's not better to make more breaking changes in one go? I'm worried that going through another great migration would agitate some.
I previously thought that too – that we should figure out the correct interfaces and write good migration documentation and tools and get all the breaking changes over with in one go – but the end result was that this work stalled out for years because there’s a lot of API design and implementation work to get things to a good state: we need to decide which things should be global or per‐user; we need to decide what should stay in nix-darwin and what should be deduplicated with Home Manager; for things like our If we tried to do it all at once, this work would have spent even longer in development hell and be unreviewably big. For the longest time i wasn’t sure how to resolve that, but after discussing on Matrix I realized that introducing a In the case of Homebrew, |
229a7c6
to
a387b4b
Compare
Most of the system.defaults stuff should really move to home-manager anyway, as you said. It's a natural fit and it works well. |
I've been running this branch on my machine for the past week and had zero issues with it, thanks for these great improvements! |
Been running this for a few days since I got back from vacation. Noticing issues with nix store on reboots, haven't pinned down what's going on atm. Not sure if I need to do something different, I just tested copying the branch and rebasing it on master. Running my normal rebuild mentions the sudo requirement, doing a sudo rebuild mentions I need a primary user, adding a primary user and then it rebuilds. Just noting symptoms:
2025-03-08 14:57:39.712350 <Warning> Could not find and/or execute program specified by service: 2: No such file or directory: /nix/store/fa3jr2wxgbiyk37h0imzhhj13c3p6gqv-yabai-7.1.10/bin/yabai
2025-03-08 14:57:39.712351 <Error> Service could not initialize: access(/nix/store/fa3jr2wxgbiyk37h0imzhhj13c3p6gqv-yabai-7.1.10/bin/yabai, X_OK) failed with errno 2 - No such file or directory, error 0x6f - Invalid or missing Program/ProgramArguments
2025-03-08 14:57:39.712352 <Error> initialization failure: 24D70: xpcproxy + 36768 [1088][E4C3A3C3-0D57-3E4D-8388-880BC7F5F19E]: 0x6f
2025-03-08 14:57:39.712353 <Notice> Service setup event to handle failure and will not launch until it fires.
2025-03-08 14:57:39.712354 <Error> Missing executable detected. Job: 'org.nixos.yabai' Executable: '/nix/store/fa3jr2wxgbiyk37h0imzhhj13c3p6gqv-yabai-7.1.10/bin/yabai' |
That’s very confusing. I’m not sure how that could result from these changes. Are you sure you get the same result if you run the base commit that you rebased on top of? Were there any conflicts during the rebase that you had to resolve? |
Yeah, sorry... not a lot of information, I know... Probably can just disregard it for now... I'll try bisecting and see where it might have come from. Even switching back to master I get the behavior still. |
This is the exact reason |
This is using the nix-darwin service. I do see that it doesn't contain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's finally happening! This looks really good in general — left a couple notes and questions but I think many of these changes may just have to be breaking.
@@ -1,6 +1,8 @@ | |||
{ config, pkgs, lib, ... }: | |||
|
|||
{ | |||
system.primaryUser = "test-defaults-user"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same concern about the user needing to exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Same answer.)
config.nix.enable | ||
&& !config.nixpkgs.flake.setNixPath | ||
&& config.system.stateVersion < 6 | ||
&& options.environment.darwinConfig.highestPrio == (mkOptionDefault {}).priority |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hate that I can't think of a better way to check that this option wasn't changed from the default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only way I've done it, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it’s awful and I hate it. But I hate not giving good error messages when doing complicated migrations even more.
default = | ||
config.users.users.${config.system.primaryUser}.home or "/Users/${config.system.primaryUser}"; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this break anything if config.system.primaryUser
is set to null
(the default)? If I'm reading it correctly, it's only an issue if primaryUserHome
is read by something, in which case primaryUser
needs to be set anyway, but I wonder what the error message would look like. Maybe it's caught by nix.nixPath
requiring primaryUser
to be set? Not sure that works for environment.darwinConfig
though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’ll be an evaluation error if anything depends on it. But, yes, that stuff should set system.requiresPrimaryUser
(as environment.darwinConfig
does) and therefore hopefully give a helpful error message before anything that uses it would get evaluated. "${null}"
is also an error so I’m not sure allowing it to be null
would improve anything. Maybe this could be a more descriptive throw
in that case? But I haven’t triggered it so far in testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
lib.warnIf (config.system.primaryUser == null) "`system.primaryUser` is unset but `system.primaryUserHome` is being read" config.users.users.${config.system.primaryUser}.home or "/Users/${config.system.primaryUser}";
?
On the other hand, it seems like this problem will only be hit due to a future mistake on our part (reading primaryUserHome
in an option without adding that option to requiresPrimaryUser
), so we could just leave this as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making it a throwIf
might be a good idea, and seems pretty harmless. I’ll think about it. The main thing is that I wish we could make incorrect uses of config.system.primaryUser
fail loudly in the same way, but that’s harder.
system.activationScripts.postActivation.text = mkAfter '' | ||
nix-channel --remove darwin || true | ||
${optionalString (config.system.primaryUser != null) '' | ||
sudo \ | ||
--user=${config.system.primaryUser} \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not shell escaping the primary user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly anyone who tries to set a username that requires shell escaping has it coming. But, yeah, fixed.
@@ -65,7 +65,7 @@ in | |||
ln -sfn /run/current-system /nix/var/nix/gcroots/current-system | |||
fi | |||
|
|||
${config.system.activationScripts.etcChecks.text} | |||
${config.system.activationScripts.checks.text} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The one thing I am concerned about here is if checks
has stuff that needs to be run in a graphical context. Based on a quick search through our in-tree checks it seems like this change would cause errors for the activation daemon from modules/users/default.nix
in some cases, it might cause errors for our other checks, and might for out-of-tree checks if people were assuming that checks
would only run in a graphical session.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The activation daemon only activates system configurations that were previously current. So in theory it should never be trying to delete users and so on. What other checks do you think might be problematic here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good point. I don't have any problematic checks I can think of, and they should be read-only anyway, so I'm good with this.
${cfg.activationScripts.checks.text} | ||
${cfg.activationScripts.createRun.text} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also a potential point where out-of-tree checks could break, if people assume that /run
will already exist. I think this is still a worthwhile change to make, but worth pointing out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could put it before checks
if this is considered a big problem; I just figured it’s best to change the system as little as possible if any checks are failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is definitely an improvement that we should make, just noting the potential issue here in case we see it in the future.
a387b4b
to
0d9daef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that covers everything I saw! Another pair of eyes certainly wouldn't hurt but this LGTM
Hey, I've just not long ago moved to flakes and don't have a lot of experience here. Could someone give me rough instructions how I would test it with flake setup? I would like to help 🙂 based on what I am seeing in the PR already started migrating away from services and config options which require primaryUser. |
You can test it by adding the following snippet to your flake: inputs.nix-darwin = {
url = "github:lnl7/nix-darwin/pull/1341/merge";
inputs.nixpkgs.follows = "nixpkgs"; # if you want to lock it to the same rev as your main nixpkgs instance
}; |
So it is a bit strange for me with launch agents 🙂 I've had couple launch agents defined via
|
3 sounds like expected behavior. 1, unfortunately, also sounds like expected behavior, since the code to handle removing those LaunchAgents has itself been removed. 2 is surprising to me though, unless the LaunchAgent plists are put at a different path? |
Rebased. I’m expecting this to probably not land before we do the repository move, although it’s still my highest‐priority change to the actual codebase. It would be great if someone had the time to test I’m wondering if it sense to rename the activation script options while doing this, to pave the way for safe custom activation scripts that never overlap with NixOS (see the tragically‐delayed #664) and to avoid situations where someone has something in @retrry Yes, this generally sounds like the typical unfortunate behaviour we currently have whenever we stop tracking files. I am not sure if there is much elegant we can do to avoid that right now. In the future @Samasaur1’s nice file tracking stuff would remove the files in (1) there. Maybe it would make sense to land that functionality before this, although I expect that the majority of people will just set a primary user at first rather than trying to de‐user‐ify their configuration upfront. I am indeed surprised by (2) though and maybe we should figure that out. Were you setting the same primary user as the one that was running |
These can’t be relied upon in a post‐user‐activation world. Technically a breaking change, if anyone has their home directory outside of `/Users` or is using `root` for this, but, well, I did my best and these are legacy defaults anyway.
The `activate-system` daemon will now run all the checks, which seems like probably a good idea anyway?
The checks should no longer depend on `/run`, so this avoids modifying the system before they run.
640899e
to
b9e580c
Compare
Thanks to @Enzime for helping push this over the finish line. I think we should be ready to merge this and branch off 25.05 once a couple critical downstreams (nix-homebrew, nh, maybe others?) merge support for it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be good to go now 👍
My PR to |
Okay let’s do this. Sorry if this breaks your system. |
nh 4.0+ is in the unstable branch, but it was not backported to stable due to a small regression in Darwin. I'll probably finish the PR that fixes the Darwin regression and does some internal cleanup shortly after 25.05 releases, and make a tag for 4.1.0. |
As of ~a week ago, user activation is no longer allowed and system activation must be run as root. See nix-darwin/nix-darwin#1341 for details; in particular, the first commit is a changelog entry. I'm pretty sure I don't use any of the options affected here.
Since [1] [2], nix-darwin requires using "sudo" when invoking "darwin-rebuild" and "nix-channel" commands. This also means that our custom "HOME" variable cannot be used as is because the "HOME" will point to the root user (/private/var/root). So instead we have to use "config.system.primaryUserHome" instead, after defining "system.primaryUser". In summary, we had to: 1) Add the darwin, nixpkgs, and nixpkgs-unstable channels (we reference nixpkgs-stable in our config) with "sudo nix-channel --add". These are the channels we used: $ sudo nix-channel --list warning: $HOME ('/Users/l') is not owned by you, falling back to the one defined in the 'passwd' file ('/var/root') darwin https://github.com/nix-darwin/nix-darwin/archive/nix-darwin-25.05.tar.gz nixpkgs https://nixos.org/channels/nixpkgs-25.05-darwin nixpkgs-unstable https://nixos.org/channels/nixpkgs-unstable 2) Run "sudo darwin-rebuild switch -I darwin-config=/Users/l/syscfg/nixpkgs/darwin-configuration.nix" because for whatever reason darwin-rebuild could not find our darwin-configuration.nix even though we had it inside our NIX_PATH: $ echo $NIX_PATH /Users/l/.nix-defexpr/channels:darwin-config=/Users/l/.nixpkgs/darwin-configuration.nix:/nix/var/nix/profiles/per-user/root/channels It looks like passing the "-I" flag to darwin-rebuild" is required only once (although this "only required once" language was removed in [3]). Anyway, this proves that we can use the channel-based nix-darwin setup with the new move to requiring "sudo" for interacting with nix-darwin. It looks like documentation is not up to date though for brand new installs though [4]. [1] nix-darwin/nix-darwin#1341 [2] https://github.com/nix-darwin/nix-darwin/blob/fa6120c32f10bd2aac9e8c9a6e71528a9d9d823b/modules/system/primary-user.nix#L53-L58 [3] nix-darwin/nix-darwin@af62c4d [4] nix-darwin/nix-darwin#1434
It’s finally time.
This PR eliminates user activation, addressing #96 by following the first phase of the plan from #1016 (comment), fulfilling a goal I started drafting patches for in 2023 before I was even a nix-darwin maintainer.
I strongly recommend reviewing this one commit‐by‐commit. See the commit messages for much more detail and for things I’m still uncertain about. I’ve been running this on my own system for a while now, but it could definitely use a staged roll‐out before we actually hit the merge button. We should also probably also reach out to authors of third‐party modules that will be affected once it’s mostly baked, though at least anything that breaks should mostly break loudly. (Adding text to our existing
activationScripts.{checks,userDefaults,userLaunchd,homebrew}.text
definitions is an exception – those will run asroot
now. But I can’t think of a particularly graceful way to handle that case, and anyway it’s not exactly something we fully support in the first place.)Closes: #96